Azure Communication Service - Phone Number Administration SDK #15497
Conversation
waynemo
requested review from
JianpingChen,
JonathanGiles,
RezaJooyandeh and
mssfang
as code owners
...istration/src/main/java/com/azure/communication/administration/PhoneNumberClientBuilder.java
Show resolved
Hide resolved
...n/java/com/azure/communication/administration/implementation/PhoneNumberAdminClientImpl.java
Show resolved
Hide resolved
| * @return A {@link Mono} containing a {@link AreaCodes} representing area codes. | ||
| */ | ||
| @ServiceMethod(returns = ReturnType.SINGLE) | ||
| public Mono<AreaCodes> getAllAreaCodes( |
There was a problem hiding this comment.
Please ensure that none of these methods throw exception. Exception should be returned through Mono.error or Flux.error.
There was a problem hiding this comment.
Perhaps we can update the error handling in a separate PR.
| import java.util.stream.Collectors; | ||
|
|
||
| /** | ||
| * Asynchronous client for Communication service phone number operations |
There was a problem hiding this comment.
In general, our javadocs should also include codesnippets on how to instantiate the client at the class level and how to use the APIs for each of the public methods.
There was a problem hiding this comment.
Let's add this in a separate PR.
| public Mono<UpdateNumberCapabilitiesResponse> updateCapabilities( | ||
| Map<PhoneNumber, NumberUpdateCapabilities> phoneNumberCapabilitiesUpdate) { | ||
| Map<String, NumberUpdateCapabilities> capabilitiesMap = new HashMap<>(); | ||
| for (Map.Entry<PhoneNumber, NumberUpdateCapabilities> entry : phoneNumberCapabilitiesUpdate.entrySet()) { |
There was a problem hiding this comment.
Add null checks before using user-input (phoneNumberCapabilitiesUpdate) and provide helpful error message. Do the same for other methods too.
Objects.requireNonNull(phoneNumberCapabilitiesUpdate, "'phoneNumberCapabilitiesUpdate' cannot be null");
There was a problem hiding this comment.
Perhaps we can update the error handling in a separate PR.
| private final PhoneNumberAdminClientImpl phoneNumberAdminClient; | ||
|
|
||
| PhoneNumberAsyncClient(PhoneNumberAdminClientImpl phoneNumberAdminClient) { | ||
| this.phoneNumberAdminClient = phoneNumberAdminClient; |
There was a problem hiding this comment.
Looks like all APIs in this class only uses phoneNumberAdminClient.getAdministrations(). So, it might be simpler to have the Administrations type as instance variable.
There was a problem hiding this comment.
Good idea. Will update.
| * @return The updated {@link PhoneNumberClientBuilder} object. | ||
| */ | ||
| public PhoneNumberClientBuilder pipeline(HttpPipeline pipeline) { | ||
| this.pipeline = Objects.requireNonNull(pipeline, "'pipeline' cannot be null."); |
There was a problem hiding this comment.
This can be null in scenarios where the user initially set the pipeline but then decided to clear it out. These validations can be postponed until the build method is called.
There was a problem hiding this comment.
To be addressed in separate PR.
...istration/src/main/java/com/azure/communication/administration/PhoneNumberClientBuilder.java
Show resolved
Hide resolved
...src/test/java/com/azure/communication/administration/PhoneNumberAsyncClientPlaybackTest.java
Outdated
Show resolved
Hide resolved
...src/test/java/com/azure/communication/administration/PhoneNumberAsyncClientPlaybackTest.java
Outdated
Show resolved
Hide resolved
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| @Execution(value = ExecutionMode.SAME_THREAD) |
There was a problem hiding this comment.
It's needed for mocking in the unit tests. If the tests are run in parallel, the mocks will have conflicts.
…15497) * Add Phone Number functionality to azure-communication-administration * Update checkstyle suppressions
* Azure Communication Service - Phone Number Administration SDK (#15497) * Add Phone Number functionality to azure-communication-administration * Update checkstyle suppressions * Azure Communication Services - Refactor PhoneNumberAsyncClient to Wrap Exceptions (#15794) * Catch exceptions and return error Mono/PagedFlux * Refactor PhoneNumberAsyncClient method overloads to consolidate logic * Azure Communication Service - Update Phone Number Admin Null Checks (#15811) * Add null checks for required method arguments * Remove setter null check to allow clearing of httpPipeline * Updating azure-communication-administration README with a few samples (#15816) * Updating javadoc with a few samples * resolve comments * Fixing spot check errors * fix typo * Add Env Var check to skip PhoneNumber integration tests (#15855) * Samples and Readme update for Administration package (#15853) * More samples and readme changes * PR comments * build failure * fix indentation * azure-communication-administration - Regenerate Code for Updated Swagger (#15861) * Update swagger and regenerate code * Fix tests after code regeneration * Fix the param name * updating the sample code Co-authored-by: waynemo <[email protected]>
4 participants
This PR adds the initial Phone Number Administration SDK. It includes the following:
To be completed in separate PRs: